-
Notifications
You must be signed in to change notification settings - Fork 87
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
<username>-che as default namespace #156
Conversation
… to che serviceaccount Signed-off-by: Michal Vala <mvala@redhat.com>
Signed-off-by: Michal Vala <mvala@redhat.com>
…amespaces Signed-off-by: Michal Vala <mvala@redhat.com>
…rent namespace than che Signed-off-by: Michal Vala <mvala@redhat.com>
Signed-off-by: Michal Vala <mvala@redhat.com>
Signed-off-by: Michal Vala <mvala@redhat.com>
Fix failing workspace start due 'update' namespace permission. Handle todo about saving Che worksapce cluster role property to config map. Rename che cluster role to che-manage-namespaces Signed-off-by: Oleksandr Andriienko <oandriie@redhat.com>
Signed-off-by: Oleksandr Andriienko <oandriie@redhat.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
few nitpicks
deploy_k8s.sh
Outdated
# sometimes the operator cannot get CRD right away | ||
sleep 2 | ||
|
||
# uncomment if you need Login with OpenShift |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is irrelevant here
deploy_k8s.sh
Outdated
|
||
BASE_DIR=$(cd "$(dirname "$0")"; pwd) | ||
|
||
kubectl apply -f "${BASE_DIR}"/deploy/service_account.yaml -n che |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would be nice if script accepts one arg with namespace, defaulting to che
pkg/controller/che/che_controller.go
Outdated
} | ||
|
||
// this binding is needed to manage che workspaces out of che namespace | ||
// `che` ClusterRole should be created during che-operator deploy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the comment is no longer valid. you're binding che-manage-namespace
role
pkg/controller/che/che_controller.go
Outdated
} | ||
} else { | ||
// this binding is needed to create new namespaces for workspaces | ||
// `che` ClusterRole should be created during che-operator deploy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the comment is no longer valid. you're binding che-create-namespaces
role
pkg/deploy/che_configmap.go
Outdated
@@ -164,6 +166,7 @@ func GetConfigMapData(cr *orgv1.CheCluster) (cheEnv map[string]string) { | |||
CheDebugServer: cheDebug, | |||
CheInfrastructureActive: infra, | |||
CheInfraKubernetesServiceAccountName: "che-workspace", | |||
CheWorkspaceClusterRole: cheWorkspaceClusterRole, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
formatting looks bit off here
When used with chectl, I'm getting:
you can try with
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
currently does not work with chectl
It looks like the patch is not actually changing default namespace. It has to be defined in |
pr for chectl che-incubator/chectl#469 . @sparkoo Did you tested this one? |
no :) will do |
works with che-incubator/chectl#469. Just need to properly handle templates. |
…the minikube: create own unique clusterrole and clusterrolebinding for Che server. We need it if we want to have working few che in the same cluster in the different namespaces. Signed-off-by: Oleksandr Andriienko <oandriie@redhat.com>
Can one of the admins verify this patch? |
Signed-off-by: Oleksandr Andriienko <oandriie@redhat.com>
…r roles Signed-off-by: Oleksandr Andriienko <oandriie@redhat.com>
Hello, @davidfestal, @tolusha , @mmorhun . I completed testing current pr with OLM on the minikube and CRC clutster. Looks like everything works just fine. Could you review pr, please? |
} | ||
|
||
// this binding is needed to manage che workspaces out of che namespace | ||
// `${namespace}-clusterrole-manage-namespaces` ClusterRole should be created during che-operator deploy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If ClusterRole is created during che-operator deploy, like this comment say, why it is created again here https://github.com/eclipse/che-operator/pull/156/files#diff-d9445e614f0d728fd140771b0fd29c50R1315 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OLM auto-generate unique names for che-operator ClusterRoles. ClusterRole object is not namespaced it has wider scope. That's why olm generate name to have ability to use few operators in the same cluster with own specific version of the ClusterRoles and their rules. We can not find and reuse this roles for che-server by name, That's why I created for che-server own Cluster roles.
P.S.: Theoretically we can reuse che-operator cluster role using labels, but I used current approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, ok. thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nickboldt @l0rd is it planned to have this change in 7.9.0 ? this would affect defaults for CRW I think
@@ -131,6 +131,7 @@ Or you can build in a container: | |||
docker run -ti -v /tmp:/tmp -v ${OPERATOR_REPO}:/opt/app-root/src/go/src/github.com/eclipse/che-operator registry.redhat.io/devtools/go-toolset-rhel7:1.11.5-3 sh -c "OOS=linux GOARCH=amd64 CGO_ENABLED=0 go build -o /tmp/run-tests /opt/app-root/src/go/src/github.com/eclipse/che-operator/e2e/*.go" | |||
cp /tmp/run-tests ${OPERATOR_REPO}/run-tests | |||
``` | |||
> Notice: if you don't have redhat subscription, use public image registry.access.redhat.com/devtools/go-toolset-rhel7:latest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is not right that we have links to private images in Che project.
But this is out of scope of this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Tested with default che
namespace and everything worked for me.
Sorry guys, pr moved #166, because Ci become crazy with branch name pr/137 ... And github doesn't provide ability to rename target branch for pr. I can change base branch, but not target. |
Referenced issue
issue: eclipse-che/che#15493
What does this PR do?
Continue work started #137 :
workspaceNamespaceDefault
)che
sa with no granted permissionsRelated pr(s)
che-incubator/chectl#469
TODO:
che
sa